-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce FormattingOptions to 64 bits #136974
base: master
Are you sure you want to change the base?
Conversation
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Reduce FormattingOptions to 64 bits This reduces FormattingOptions from 6-7 machine words (384 bits on 64-bit platforms, 224 bits on 32-bit platforms) to just 64 bits (a single register on 64-bit platforms). This PR includes rust-lang#136932, which reduces the width and precision options to 16 bits, to make it all fit. Before: ```rust pub struct FormattingOptions { flags: u32, // only 6 bits used fill: char, align: Option<Alignment>, width: Option<usize>, precision: Option<usize>, } ``` After: ```rust pub struct FormattingOptions { /// Bits: /// - 0: `+` flag [rt::Flag::SignPlus] /// - 1: `-` flag [rt::Flag::SignMinus] /// - 2: `#` flag [rt::Flag::Alternate] /// - 3: `0` flag [rt::Flag::SignAwareZeroPad] /// - 4: `x?` flag [rt::Flag::DebugLowerHex] /// - 5: `X?` flag [rt::Flag::DebugUpperHex] /// - 6-7: Alignment (0: Left, 1: Right, 2: Center, 3: Unknown) /// - 8: Width flag (if set, the width field below is used) /// - 9: Precision flag (if set, the precision field below is used) /// - 10: unused /// - 11-31: fill character (21 bits, a full `char`) flags: u32, /// Width if width flag above is set. Otherwise, always 0. width: u16, /// Precision if precision flag above is set. Otherwise, always 0. precision: u16, } ```
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (47853a6): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 2.0%, secondary -1.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary -0.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary -0.2%, secondary -0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 792.804s -> 787.91s (-0.62%) |
Looks fine. Binary size improvements looking good too: https://perf.rust-lang.org/compare.html?start=54cdc751df770517e70db0588573e32e6a7b9821&end=47853a630ac42e5c5ab5fcd1f07d37eef67f84e9&stat=size%3Alinked_artifact&tab=compile |
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
4700995
to
dd37150
Compare
This comment was marked as outdated.
This comment was marked as outdated.
…=scottmcm Reduce formatting `width` and `precision` to 16 bits This is part of rust-lang#99012 This is reduces the `width` and `precision` fields in format strings to 16 bits. They are currently full `usize`s, but it's a bit nonsensical that we need to support the case where someone wants to pad their value to eighteen quintillion spaces and/or have eighteen quintillion digits of precision. By reducing these fields to 16 bit, we can reduce `FormattingOptions` to 64 bits (see rust-lang#136974) and improve the in memory representation of `format_args!()`. (See additional context below.) This also fixes a bug where the width or precision is silently truncated when cross-compiling to a target with a smaller `usize`. By reducing the width and precision fields to the minimum guaranteed size of `usize`, 16 bits, this bug is eliminated. This is a breaking change, but affects almost no existing code. --- Details of this change: There are three ways to set a width or precision today: 1. Directly a formatting string, e.g. `println!("{a:1234}")` 2. Indirectly in a formatting string, e.g. `println!("{a:width$}", width=1234)` 3. Through the unstable `FormattingOptions::width` method. This PR: - Adds a compiler error for 1. (`println!("{a:9999999}")` no longer compiles and gives a clear error.) - Adds a runtime check for 2. (`println!("{a:width$}, width=9999999)` will panic.) - Changes the signatures of the (unstable) `FormattingOptions::[get_]width` methods to use a `u16` instead. --- Additional context for improving `FormattingOptions` and `fmt::Arguments`: All the formatting flags and options are currently: - The `+` flag (1 bit) - The `-` flag (1 bit) - The `#` flag (1 bit) - The `0` flag (1 bit) - The `x?` flag (1 bit) - The `X?` flag (1 bit) - The alignment (2 bits) - The fill character (21 bits) - Whether a width is specified (1 bit) - Whether a precision is specified (1 bit) - If used, the width (a full usize) - If used, the precision (a full usize) Everything except the last two can simply fit in a `u32` (those add up to 31 bits in total). If we can accept a max width and precision of u16::MAX, we can make a `FormattingOptions` that is exactly 64 bits in size; the same size as a thin reference on most platforms. If, additionally, we also limit the number of formatting arguments, we can also reduce the size of `fmt::Arguments` (that is, of a `format_args!()` expression).
…=scottmcm Reduce formatting `width` and `precision` to 16 bits This is part of rust-lang#99012 This is reduces the `width` and `precision` fields in format strings to 16 bits. They are currently full `usize`s, but it's a bit nonsensical that we need to support the case where someone wants to pad their value to eighteen quintillion spaces and/or have eighteen quintillion digits of precision. By reducing these fields to 16 bit, we can reduce `FormattingOptions` to 64 bits (see rust-lang#136974) and improve the in memory representation of `format_args!()`. (See additional context below.) This also fixes a bug where the width or precision is silently truncated when cross-compiling to a target with a smaller `usize`. By reducing the width and precision fields to the minimum guaranteed size of `usize`, 16 bits, this bug is eliminated. This is a breaking change, but affects almost no existing code. --- Details of this change: There are three ways to set a width or precision today: 1. Directly a formatting string, e.g. `println!("{a:1234}")` 2. Indirectly in a formatting string, e.g. `println!("{a:width$}", width=1234)` 3. Through the unstable `FormattingOptions::width` method. This PR: - Adds a compiler error for 1. (`println!("{a:9999999}")` no longer compiles and gives a clear error.) - Adds a runtime check for 2. (`println!("{a:width$}, width=9999999)` will panic.) - Changes the signatures of the (unstable) `FormattingOptions::[get_]width` methods to use a `u16` instead. --- Additional context for improving `FormattingOptions` and `fmt::Arguments`: All the formatting flags and options are currently: - The `+` flag (1 bit) - The `-` flag (1 bit) - The `#` flag (1 bit) - The `0` flag (1 bit) - The `x?` flag (1 bit) - The `X?` flag (1 bit) - The alignment (2 bits) - The fill character (21 bits) - Whether a width is specified (1 bit) - Whether a precision is specified (1 bit) - If used, the width (a full usize) - If used, the precision (a full usize) Everything except the last two can simply fit in a `u32` (those add up to 31 bits in total). If we can accept a max width and precision of u16::MAX, we can make a `FormattingOptions` that is exactly 64 bits in size; the same size as a thin reference on most platforms. If, additionally, we also limit the number of formatting arguments, we can also reduce the size of `fmt::Arguments` (that is, of a `format_args!()` expression).
…=scottmcm Reduce formatting `width` and `precision` to 16 bits This is part of rust-lang#99012 This is reduces the `width` and `precision` fields in format strings to 16 bits. They are currently full `usize`s, but it's a bit nonsensical that we need to support the case where someone wants to pad their value to eighteen quintillion spaces and/or have eighteen quintillion digits of precision. By reducing these fields to 16 bit, we can reduce `FormattingOptions` to 64 bits (see rust-lang#136974) and improve the in memory representation of `format_args!()`. (See additional context below.) This also fixes a bug where the width or precision is silently truncated when cross-compiling to a target with a smaller `usize`. By reducing the width and precision fields to the minimum guaranteed size of `usize`, 16 bits, this bug is eliminated. This is a breaking change, but affects almost no existing code. --- Details of this change: There are three ways to set a width or precision today: 1. Directly a formatting string, e.g. `println!("{a:1234}")` 2. Indirectly in a formatting string, e.g. `println!("{a:width$}", width=1234)` 3. Through the unstable `FormattingOptions::width` method. This PR: - Adds a compiler error for 1. (`println!("{a:9999999}")` no longer compiles and gives a clear error.) - Adds a runtime check for 2. (`println!("{a:width$}, width=9999999)` will panic.) - Changes the signatures of the (unstable) `FormattingOptions::[get_]width` methods to use a `u16` instead. --- Additional context for improving `FormattingOptions` and `fmt::Arguments`: All the formatting flags and options are currently: - The `+` flag (1 bit) - The `-` flag (1 bit) - The `#` flag (1 bit) - The `0` flag (1 bit) - The `x?` flag (1 bit) - The `X?` flag (1 bit) - The alignment (2 bits) - The fill character (21 bits) - Whether a width is specified (1 bit) - Whether a precision is specified (1 bit) - If used, the width (a full usize) - If used, the precision (a full usize) Everything except the last two can simply fit in a `u32` (those add up to 31 bits in total). If we can accept a max width and precision of u16::MAX, we can make a `FormattingOptions` that is exactly 64 bits in size; the same size as a thin reference on most platforms. If, additionally, we also limit the number of formatting arguments, we can also reduce the size of `fmt::Arguments` (that is, of a `format_args!()` expression).
Reduce FormattingOptions to 64 bits This is part of rust-lang#99012 This reduces FormattingOptions from 6-7 machine words (384 bits on 64-bit platforms, 224 bits on 32-bit platforms) to just 64 bits (a single register on 64-bit platforms). Before: ```rust pub struct FormattingOptions { flags: u32, // only 6 bits used fill: char, align: Option<Alignment>, width: Option<usize>, precision: Option<usize>, } ``` After: ```rust pub struct FormattingOptions { /// Bits: /// - 0-20: fill character (21 bits, a full `char`) /// - 21: `+` flag /// - 22: `-` flag /// - 23: `#` flag /// - 24: `0` flag /// - 25: `x?` flag /// - 26: `X?` flag /// - 27: Width flag (if set, the width field below is used) /// - 28: Precision flag (if set, the precision field below is used) /// - 29-30: Alignment (0: Left, 1: Right, 2: Center, 3: Unknown) /// - 31: Always set to 1 flags: u32, /// Width if width flag above is set. Otherwise, always 0. width: u16, /// Precision if precision flag above is set. Otherwise, always 0. precision: u16, } ```
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (799f4a4): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 1.2%, secondary 2.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary -2.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary -0.2%, secondary -0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 779.458s -> 780.195s (0.09%) |
…cottmcm Reduce formatting `width` and `precision` to 16 bits This is part of rust-lang#99012 This is reduces the `width` and `precision` fields in format strings to 16 bits. They are currently full `usize`s, but it's a bit nonsensical that we need to support the case where someone wants to pad their value to eighteen quintillion spaces and/or have eighteen quintillion digits of precision. By reducing these fields to 16 bit, we can reduce `FormattingOptions` to 64 bits (see rust-lang#136974) and improve the in memory representation of `format_args!()`. (See additional context below.) This also fixes a bug where the width or precision is silently truncated when cross-compiling to a target with a smaller `usize`. By reducing the width and precision fields to the minimum guaranteed size of `usize`, 16 bits, this bug is eliminated. This is a breaking change, but affects almost no existing code. --- Details of this change: There are three ways to set a width or precision today: 1. Directly a formatting string, e.g. `println!("{a:1234}")` 2. Indirectly in a formatting string, e.g. `println!("{a:width$}", width=1234)` 3. Through the unstable `FormattingOptions::width` method. This PR: - Adds a compiler error for 1. (`println!("{a:9999999}")` no longer compiles and gives a clear error.) - Adds a runtime check for 2. (`println!("{a:width$}, width=9999999)` will panic.) - Changes the signatures of the (unstable) `FormattingOptions::[get_]width` methods to use a `u16` instead. --- Additional context for improving `FormattingOptions` and `fmt::Arguments`: All the formatting flags and options are currently: - The `+` flag (1 bit) - The `-` flag (1 bit) - The `#` flag (1 bit) - The `0` flag (1 bit) - The `x?` flag (1 bit) - The `X?` flag (1 bit) - The alignment (2 bits) - The fill character (21 bits) - Whether a width is specified (1 bit) - Whether a precision is specified (1 bit) - If used, the width (a full usize) - If used, the precision (a full usize) Everything except the last two can simply fit in a `u32` (those add up to 31 bits in total). If we can accept a max width and precision of u16::MAX, we can make a `FormattingOptions` that is exactly 64 bits in size; the same size as a thin reference on most platforms. If, additionally, we also limit the number of formatting arguments, we can also reduce the size of `fmt::Arguments` (that is, of a `format_args!()` expression).
Reduce formatting `width` and `precision` to 16 bits This is part of rust-lang/rust#99012 This is reduces the `width` and `precision` fields in format strings to 16 bits. They are currently full `usize`s, but it's a bit nonsensical that we need to support the case where someone wants to pad their value to eighteen quintillion spaces and/or have eighteen quintillion digits of precision. By reducing these fields to 16 bit, we can reduce `FormattingOptions` to 64 bits (see rust-lang/rust#136974) and improve the in memory representation of `format_args!()`. (See additional context below.) This also fixes a bug where the width or precision is silently truncated when cross-compiling to a target with a smaller `usize`. By reducing the width and precision fields to the minimum guaranteed size of `usize`, 16 bits, this bug is eliminated. This is a breaking change, but affects almost no existing code. --- Details of this change: There are three ways to set a width or precision today: 1. Directly a formatting string, e.g. `println!("{a:1234}")` 2. Indirectly in a formatting string, e.g. `println!("{a:width$}", width=1234)` 3. Through the unstable `FormattingOptions::width` method. This PR: - Adds a compiler error for 1. (`println!("{a:9999999}")` no longer compiles and gives a clear error.) - Adds a runtime check for 2. (`println!("{a:width$}, width=9999999)` will panic.) - Changes the signatures of the (unstable) `FormattingOptions::[get_]width` methods to use a `u16` instead. --- Additional context for improving `FormattingOptions` and `fmt::Arguments`: All the formatting flags and options are currently: - The `+` flag (1 bit) - The `-` flag (1 bit) - The `#` flag (1 bit) - The `0` flag (1 bit) - The `x?` flag (1 bit) - The `X?` flag (1 bit) - The alignment (2 bits) - The fill character (21 bits) - Whether a width is specified (1 bit) - Whether a precision is specified (1 bit) - If used, the width (a full usize) - If used, the precision (a full usize) Everything except the last two can simply fit in a `u32` (those add up to 31 bits in total). If we can accept a max width and precision of u16::MAX, we can make a `FormattingOptions` that is exactly 64 bits in size; the same size as a thin reference on most platforms. If, additionally, we also limit the number of formatting arguments, we can also reduce the size of `fmt::Arguments` (that is, of a `format_args!()` expression).
precision: u16, | ||
} | ||
|
||
// This needs to match with compiler/rustc_ast_lowering/src/format.rs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pondering: if these absolutely have to match, would it be worth exposing them as an unstable feature? Or something weirder like a shared include!
file?
That said the packing looks like it's basically out of bits, so it's probably not going to change...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exposing them as an unstable feature?
That'd mean that rustc uses the constants from stage 0 std, which will need to match with stage 0 rustc, resulting in nightmares.
Or something weirder like a shared include! file?
Probably not worth it. It's not just the constants that need to match. The meaning of them and the rest of the structure also needs to match, which is hard to express in a few constants.
If we ever change this, we're not just going to swap a few bits around. We'll be changing the rest of the structure too, probably, requiring more code changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more use case for #114209
/// │ ├─ 1: Align right. (>) | ||
/// │ ├─ 2: Align center. (^) | ||
/// │ └─ 3: Alignment not set. (default) | ||
/// └─ Always set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pondering: would it be worth storing the flags in a type that exposes this information to the niche calculations?
Defining such a type would just be another line in
rust/library/core/src/num/niche_types.rs
Line 156 in 70237a8
pub struct U32NotAllOnes(u32 as u32 in 0..=0xffff_fffe); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be interesting. With the highest bit always set, a few mutually exclusive flags, and the char
in the lowest 21 bits, we could use a range of 0x8000_0000..=0xfdd0ffff
.
Not sure if that's very useful though. I don't think this is a type that is commonly used in an Option or other enum. 🤔
I'll add a TODO comment for now, so we can explore it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good point. I hadn't thought that the max could also be more specific.
But yeah, later is fine.
// Extract the debug upper/lower hex, zero pad, alternate, and plus/minus flags | ||
// to stay compatible with older versions of Rust. | ||
self.options.flags >> 21 & 0x3F |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking: do we have pinning tests already that continue to pass after this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there's currently no test for this method. This method is deprecated, so perhaps we shouldn't put effort into trying to keep it compatible with older versions of Rust.
Maybe we should add a test, or change the method to just always return 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test.
Looks good to me! I put some thoughts above, but |
This comment has been minimized.
This comment has been minimized.
c5f6ecf
to
d7685f0
Compare
@bors r=scottmcm |
Reduce FormattingOptions to 64 bits This is part of rust-lang#99012 This reduces FormattingOptions from 6-7 machine words (384 bits on 64-bit platforms, 224 bits on 32-bit platforms) to just 64 bits (a single register on 64-bit platforms). Before: ```rust pub struct FormattingOptions { flags: u32, // only 6 bits used fill: char, align: Option<Alignment>, width: Option<usize>, precision: Option<usize>, } ``` After: ```rust pub struct FormattingOptions { /// Bits: /// - 0-20: fill character (21 bits, a full `char`) /// - 21: `+` flag /// - 22: `-` flag /// - 23: `#` flag /// - 24: `0` flag /// - 25: `x?` flag /// - 26: `X?` flag /// - 27: Width flag (if set, the width field below is used) /// - 28: Precision flag (if set, the precision field below is used) /// - 29-30: Alignment (0: Left, 1: Right, 2: Center, 3: Unknown) /// - 31: Always set to 1 flags: u32, /// Width if width flag above is set. Otherwise, always 0. width: u16, /// Precision if precision flag above is set. Otherwise, always 0. precision: u16, } ```
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
This is part of #99012
This reduces FormattingOptions from 6-7 machine words (384 bits on 64-bit platforms, 224 bits on 32-bit platforms) to just 64 bits (a single register on 64-bit platforms).
Before:
After: